Skip to content

feature/SOF-8034 Fix: preserve the input crystal in build metadata - #297

Merged
VsevolodX merged 8 commits into
mainfrom
feature/SOF-8034
Aug 26, 2026
Merged

feature/SOF-8034 Fix: preserve the input crystal in build metadata#297
VsevolodX merged 8 commits into
mainfrom
feature/SOF-8034

Conversation

@VsevolodX

Copy link
Copy Markdown
Member

Closes the SOF-8034 defect: builders recorded a conventionalized copy of the material instead of the one passed in, so a Total Energy job run on a primitive cell was unreachable from a slab built from it. create_slab(Ni_primitive, (0,0,1), 3) recorded ad81bcd8… (conventional Ni4) instead of the input 18d50c5c….

What changed

The input crystal is now stored untransformed, and conventionalization moves to build time — where the use_conventional_cell flag already lived.

  • CrystalLatticePlanesBuilder._generate applies the transform
  • SlabConfiguration.from_parameters forwards the flag instead of rebinding material
  • four helpers (zsl, commensurate, both grain boundaries) stop conventionalizing before building their configuration
  • create_atomic_layers pins the flag False, preserving its existing behaviour

hash/scaledHash are computed fields and _id rides on the stored object, so no round-trip helper was needed — the ticket's root cause 3 turned out to be unnecessary rather than deferred.

Two named exclusions — the criterion is not "every builder"

  • twistedTwistedNanoribbonsInterfaceAnalyzer reads atomic_layers.crystal as geometry and never calls the builder, so the transform cannot reach it there. twisted/helpers.py is deliberately unchanged; an earlier revision of this branch did change it and silently collapsed use_conventional_cell=True from 8 atoms to 1. Guarded by a test now.
  • heterostructure — records metadata.build == [], on main and after this change alike. Nothing to inherit. Separate ticket.

Consequences

  • Configurations saved before this change do not round-trip. from_parameters never forwarded the flag, so every stored configuration reads "use_conventional_cell": true — including slabs built from the primitive cell. _generate used to ignore it and now honors it, so those rebuild conventionalized (measured: 3 atoms a=2.479 → 12 atoms a=3.5058). Not fixable — a stale true is indistinguishable from a real one. Pinned by a test. Rebuild such a slab from its crystal, not from its metadata.
  • Site ordering can differ on materials where conventionalization is not order-idempotent (TiN, Hf2O-MCL, Si-primitivized). Same hash, same lattice, same atom set — verified identical for TiN. This is base's redundant conventionalize² becoming conventionalize¹, not damage.
  • Newly built slabs record a different crystal hash than older ones; ~6 in-repo consumers of atomic_layers.crystal now see the input cell.

Verification

  • 313 passed (was 303 on main; +10 new) with zero existing hash fixtures edited — that was the stop condition for geometry movement
  • New tests/py/unit/test_build_metadata_records_input_crystal.py: 10 passed, and 6 failed / 3 passed against unmodified source
  • Swept every touched helper base-vs-patched across both flag values, 14 builds, comparing hash and site ordering: 13/14 byte-identical
  • surface_energy.feature passes end to end against a wheel built from this branch, served through a local JupyterLite (verified by its access log)
  • Three grounded tb-reviewer rounds; round 3 approved

Manual checks

Merging accepts anything left unticked.

  • The flag now means something. ideal_crystal/builder.py:41 and analyze/interface/twisted_nanoribbons.py:48 pass use_conventional_cell=False. Confirm their build metadata serializes false, not the schema default true.
  • Interface naming. interface/base/builder.py:105-106 derives the name from a reduced formula. Primitive Ni and Ni4 both reduce to Ni — confirm one built ZSL interface's name is byte-identical before and after.
  • lattice.type on the recorded crystal is now the input's. If the input fixture itself carries TRI for a cubic cell, that is upstream, not this task.

Not done here

calculate_interfacial_distance_from_rdf's isinstance(x, SlabConfiguration) branch returns 2.35 Å where main returned 3.85 Å for primitive Si. It has no caller in made, api-examples or the notebooks, and 2.35 Å is what its docstring promises — recorded as a finding, either delete the dead branch or give it a caller and a test.

🤖 Generated with Claude Code

Builders recorded a conventionalized copy of the material instead of the one
that was passed in, so a Total Energy job run on a primitive cell could not be
reached from a slab built from it. Store the input untransformed and move the
conventionalization to build time, where the use_conventional_cell flag already
lives.

- CrystalLatticePlanesBuilder._generate applies the transform
- SlabConfiguration.from_parameters forwards the flag instead of rebinding
- four helpers stop conventionalizing before building their configuration
- create_atomic_layers pins the flag False, preserving its behaviour

twisted is deliberately untouched: its analyzer reads atomic_layers.crystal as
geometry without building, so the transform cannot reach it there.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
VsevolodX and others added 2 commits August 24, 2026 20:41
Rewrite them the way the rest of the suite is written: call the helper and
compare against a fixture, instead of asserting invariants between two
invocations.

- one parametrized test over the six builders, comparing the recorded crystal
  to the input material
- create_atomic_layers pinned against a fixture like its siblings
- drop the twisted test: that helper is unchanged from main, so it was testing
  code this branch does not touch

167 -> 73 lines.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
use_conventional_cell was already a no-op on the commensurate and grain-boundary
paths before this branch -- the helper conventionalized, then from_parameters
conventionalized again on the schema default. Documenting that in the docstrings
of functions this branch only touches to delete a redundant transform is scope
creep, and pointing code at a ticket number for it is worse.

Those four files are now pure deletion. The dead parameter is recorded as a
follow-up finding instead.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
VsevolodX and others added 3 commits August 24, 2026 23:26
The last of the five entry points. The rebind ran before the configuration was
built, so the recorded crystal was the conventional cell rather than the input.
Both SlabConfiguration.from_parameters calls below already forwarded
use_conventional_cell, so removing the rebind is enough; the import goes with it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Nothing in the suite caught the 8-atom -> 1-atom collapse when the transform is
moved out of the twisted helper: the only existing twisted test uses graphene,
whose conventional cell is itself, so conventionalizing there is a no-op. The
regression was committed on this branch yesterday and caught by hand, not by CI.

Pin the two scalars that move. Verified: re-applying the deletion fails this test
and nothing else -- 1 failed, 310 passed.

Also: do not recurse into an already-collected crystal (an input carrying build
history would contribute the crystals nested inside it), and spell out lambda
parameters per AGENTS.md HARD RULE 4.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@VsevolodX
VsevolodX marked this pull request as ready for review August 25, 2026 07:10
Comment-only; no code changed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@VsevolodX
VsevolodX merged commit d8b2cb1 into main Aug 26, 2026
9 checks passed
@VsevolodX
VsevolodX deleted the feature/SOF-8034 branch August 26, 2026 04:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants